-
Notifications
You must be signed in to change notification settings - Fork 0
feat: implement PendingJavaScriptResult methods #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds typed result handling for JavaScript execution: new generic overloads for then(...) and toCompletableFuture(...) on ElementalPendingJavaScriptResult, a JsonCodec.decodeAs utility, and corresponding decoding support in JsonMigrationHelper25's PendingJavaScriptResultImpl. All changes focus on decoding JsonValue to requested target types. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalPendingJavaScriptResult.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonCodec.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonCodec.java (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
SuppressWarnings(149-194)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonCodec.java (1)
JsonCodec(61-104)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalPendingJavaScriptResult.java (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonCodec.java (1)
JsonCodec(61-104)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java
Show resolved
Hide resolved
bd5c2e9 to
8861333
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
171-182: Restore null‑argument validation in typedthen(...)override.The override of
then(Class<T> targetType, SerializableConsumer<T> resultHandler, ...)still omits the non‑null precondition checks that the default method provides. As a result:
resultHandler == nullwith aJsonValuetarget will now cause an asynchronousNullPointerExceptioninside the lambda.targetType == nullis only validated (if at all) by the delegate, and the behavior depends on its implementation.To keep the contract consistent and fail fast, please validate arguments before branching:
@Override @SuppressWarnings("unchecked") public <T> void then(Class<T> targetType, SerializableConsumer<T> resultHandler, SerializableConsumer<String> errorHandler) { + if (targetType == null) { + throw new IllegalArgumentException("Target type cannot be null"); + } + if (resultHandler == null) { + throw new IllegalArgumentException("Result handler cannot be null"); + } - if (targetType != null && JsonValue.class.isAssignableFrom(targetType)) { + if (JsonValue.class.isAssignableFrom(targetType)) { delegate.then(JsonNode.class, wrap(value -> { resultHandler.accept(JsonCodec.decodeAs(value, targetType)); }), errorHandler); } else { delegate.then(targetType, resultHandler, errorHandler); } }This preserves the non‑null contract and avoids deferred NPEs.
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonCodec.java (1)
43-61: Clarify class‑level Javadoc vs current implementation surface.The class Javadoc advertises a more general “encode to and from JSON” utility (including
Element/Component), but the class currently only exposesdecodeAsfor a smaller set of types. Consider narrowing the Javadoc to match what this helper actually implements, or adding the missing encode/use cases if you intend to support the full contract.src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
160-162: Avoid reusing the same generic type name for helper and outer methods.
decodeAs(JsonNode node, Class<T> type)usesTas its type parameter, which is also the type parameter name of the enclosingthen/toCompletableFuturemethods. This is legal but slightly confusing when reading call sites likedecodeAs(node, targetType). Renaming the helper’s parameter to<U>(or similar) would make the type flow clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalPendingJavaScriptResult.java(2 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonCodec.java(1 hunks)src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/flowingcode/vaadin/jsonmigration/ElementalPendingJavaScriptResult.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonCodec.java (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
SuppressWarnings(149-194)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonCodec.java (1)
JsonCodec(61-104)
🔇 Additional comments (1)
src/main/java/com/flowingcode/vaadin/jsonmigration/JsonMigrationHelper25.java (1)
184-192: Add null check fortargetTypeto matchthen()method pattern intoCompletableFuture(Class<T>).The issue is confirmed:
Class.isAssignableFrom(null)throwsNullPointerException, and line 186 calls this without a null guard. However, the inconsistency is even more evident in the codebase itself—thethen(Class<T> targetType, ...)method at line 175 already protects this exact check withif (targetType != null && JsonValue.class.isAssignableFrom(targetType)), then delegates null through to the underlying implementation (line 180).The
toCompletableFuture()method should follow the same pattern for consistency:@Override public <T> CompletableFuture<T> toCompletableFuture(Class<T> targetType) { - if (JsonValue.class.isAssignableFrom(targetType)) { + if (targetType != null && JsonValue.class.isAssignableFrom(targetType)) { return delegate.toCompletableFuture(JsonNode.class) .thenApply(node -> decodeAs(node, targetType)); } else { return delegate.toCompletableFuture(targetType); } }This prevents
NullPointerExceptionand aligns with the existing contract already demonstrated in thethen()overload.Likely an incorrect or invalid review comment.
Summary by CodeRabbit